-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inject a GetOrCreateMixin to Node and Relationship. #244
Conversation
Hi @aalekhpatel07, thank you for the contribution. We will check it asap, and get back to you. For now, can you rebase to main, since we changed our development process (I updated the Contribution guide now)? |
Update contributing guide (memgraph#245)
The changelist should be atomic and more manageable now. |
Hi @aalekhpatel07 :) I managed to look at the PR, and it all seems good. Can you maybe add a test (similar to what you provided above) here? If not, please let me add commits if you didn't allow it before. Again, thanks for the contribution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can, please add at least one test. Ideally, check the procedure both for node and relationship object.
…ementations for improved docs. Signed-off-by: Aalekh Patel <[email protected]>
Signed-off-by: Aalekh Patel <[email protected]>
Signed-off-by: Aalekh Patel <[email protected]>
I've added a couple of tests. Unfortunately, I'll let CI be the judge for these tests. |
Signed-off-by: Aalekh Patel <[email protected]>
Signed-off-by: Aalekh Patel <[email protected]>
Signed-off-by: Aalekh Patel <[email protected]>
…"name" to the node instantiation because it is a required field.
Thank you for approving the CI run. that helped spot a couple of tiny mistakes. Lets try running it again? This might be a couple of iterations since CI is the only feedback for me, but we'll get there soon. |
You can also test it locally with |
…user-defined `id`. Signed-off-by: Aalekh Patel <[email protected]>
Signed-off-by: Aalekh Patel <[email protected]>
Okay, all I went with setting up a neo4j and a memgraph instance in the background while running the tests locally: docker run \
--name neo4j \
-p 7688:7687 \
-p 7474:7474 \
-e NEO4J_AUTH="neo4j/test" \
--rm \
-it neo4j:4.4.7 docker run \
--name memgraph \
-p 7687:7687 \
-p 3000:3000 \
--rm \
-it memgraph poetry run pytest . -k 'not slow and not extras' |
Hi @aalekhpatel07, your test are passing now! Sorry for being a bit late on running them. We will push this PR to the next release. I will update you on when that is. Is our release timeline a blocker for you? |
Not really. Next release seems reasonable. It's mostly a qol improvement at this point. |
Description
Add a Django-like
get_or_create()
shortcut forNode
andRelationship
to simplify the following idiom of merging nodes and relationships:Before:
After:
Pull request type
Checklist:
######################################
Reviewer checklist (the reviewer checks this part)
######################################